improve handling of handshake failure #35549

wfurt · 2020-04-28T01:07:49Z

This is done by paying more attention to the handshake it self. In the past, we would just buffer TLS frames and pass them to OS. When handshake fail, underlying OS libraries may or may not sent Alert. With this change, we will always sent alert and we will get consistent behavior across all supported OSes. (e.g. client will always get Authentication exception instead of sometimes Authentication and sometimes IO exception)

closes #914

The other part is that sometimes we can get pretty generic (or lame) error from OS.
Example from #33186 for cipher mismatch

System.Security.Authentication.AuthenticationException: Authentication failed, see inner exception.
 ---> System.ComponentModel.Win32Exception (0x80090326): The message received was unexpected or badly formatted.
   --- End of inner exception stack trace ---

With this, we would see the Alert coming and produce something more meaningful. Type and inner exceptions are identical, but the test should now be more useful.

System.Security.Authentication.AuthenticationException: Authentication failed because the remote party has sent TLS alert ProtocolVersion.
 ---> System.ComponentModel.Win32Exception (0x80090326): The message received was unexpected or badly formatted.
   --- End of inner exception stack trace ---
   at System.Net.Security.SslStream.ForceAuthenticationAsync[TIOAdapter](TIOAdapter adapter, Boolean receiveFirst, Byte[] reAuthenticationData, Boolean isApm) in C:\Users\test\github\wfurt-runtime2\src\libraries\System.Net.Security\src\System\Net\Security\SslStream.Implementation.cs:line 282
   at Program.Main(String[] args) in C:\Users\test\source\repos\ssl\Program.cs:line 57
a

closes #33186

To make it easier, I moved some things around and I created helper to do parsing in more consolidated way. This also removes craft added to support Sslv2. It is dead for a while as none of the supported OS supports such archaic version but the code lived there never less.

more cleanup, parsing and event tracing will probably come but I felt this is good stoping point with fix for two active issues.

ghost · 2020-04-28T01:07:54Z

Tagging subscribers to this area: @dotnet/ncl
Notify danmosemsft if you want to be subscribed.

src/libraries/System.Net.Security/src/Resources/Strings.resx

wfurt · 2020-04-30T20:43:22Z

I also enabled ServerAsyncAuthenticate_MismatchProtocols_Fails. The test no longer depends on timeout/IOException and it should be more stable. (time will tell)

fixes #29642

src/libraries/System.Net.Security/src/Resources/Strings.resx

src/libraries/System.Net.Security/src/System/Net/Security/SecureChannel.cs

src/libraries/System.Net.Security/src/System/Net/Security/TlsFrameHelper.cs

stephentoub · 2020-05-05T17:30:46Z

src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNegotiatedCipherSuiteTest.cs

-        public const SslProtocols AllProtocols =
-            SslProtocols.Ssl2 | SslProtocols.Ssl3 |
+#pragma warning disable CS0618 // Ssl3 are obsolete
+        public const SslProtocols AllProtocols = SslProtocols.Ssl3 |


Why was Ssl2 removed?

Ssl2 does not have the framing protocol So determining length and type is somewhat more complicated. And I did not want to bring that to the TLS parser. With Ssl3 being deprecated and Ssl2 not supported by any of the current Ones I felt that would be ok.
rfc6176 from 2011 prohibts use of Ssl2.

Does that mean your change removes Ssl2 support from SslStream?

yes, I mention that in description. I can mark this as breaking change to make that more visible. I can also split this or roll back that part and I keep old processing around.

I think we need to have a broader conversation about that and whether we want to explicitly permanently disable SSL2 in .NET 5. On the one hand, we don't want anyone using SSL2; it's insecure, deprecated, etc. On the other hand, we know we've gotten pushback in the past because of apps talking to, e.g., services and devices that are out of their control. For the most part then we've left it up to whether the underlying operating system / platform supports it. This would be explicitly disabling such support even when it's enabled in the OS. We can choose to make that decision, but we need to do so thoughtfully.

cc: @bartonjs, @davidsh, @karelz

We shouldn't remove it completely yet from .NET 5. We still seem to run into customer issues where they have some very old device that they can't change but it talks some legacy protocol. We don't want to prevent customers from using .NET 5 due to this.

At some point, we probably would drop support from it. But I think .NET 5 is not the release to do that.

It would be interesting to get other data points such as does OpenSsl still support SSL2 in some fashion?

ok, I'll split it. maybe open new issue about retiring Sslv2 to be more explicit?

does OpenSsl still support SSL2 in some fashion?

They deleted all the SSLv2 code for OpenSSL 1.1.0. So Ubuntu 16.04 can probably still do it (OpenSSL 1.0.2), but in another year I don't think there will be any supported Linux configurations that can do SSLv2.

While Ubuntu16 has -ssl2, attempt to use it throws error so as forced run of ClientAsyncAuthenticate_Ssl2WithSelf_Success. It seems like !PlatformDetection.IsWindows10Version1607OrGreater is only viable option.
I created #35942 to track this and rolled-back Ssl2 changes.
I agree that it should not be side-effect of this change (even if convenient in this case)

src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNegotiatedCipherSuiteTest.cs

wfurt · 2020-05-07T07:10:01Z

contributes to #2359

wfurt · 2020-05-07T20:46:38Z

I rolled back all the Ssl2 changes and all tests are green.

wfurt and others added 3 commits April 27, 2020 17:05

improve handling of handshake failure

ba45dad

more cleanup

80df4d7

remove console reference

c610a8a

Dotnet-GitSync-Bot added the area-System.Net.Security label Apr 28, 2020

wfurt requested review from stephentoub and a team April 28, 2020 01:08

wfurt self-assigned this Apr 28, 2020

wfurt added this to the 5.0 milestone Apr 28, 2020

davidsh reviewed Apr 28, 2020

View reviewed changes

src/libraries/System.Net.Security/src/Resources/Strings.resx Outdated Show resolved Hide resolved

add quotes around parameter

f3c8e6d

ericstj mentioned this pull request Apr 29, 2020

Test failed: System.Text.RegularExpressions.Tests.RegexCacheTests.Ctor_Cache_Promote_entries fails with Timeout #13610

Closed

enable ServerAsyncAuthenticate_MismatchProtocols_Fails

eb6acb2

Tomas Weinfurt added 2 commits May 3, 2020 18:46

cleanup ssl2 test

db8ffba

fix http2

364093a

This was referenced May 5, 2020

Errors installing the SDK during builds #34015

Closed

OSX machines are de-provisioned during CI / PR runs leading to failures #34472

Closed

stephentoub approved these changes May 5, 2020

View reviewed changes

feedback from review

9245919

add back two missing empty lines

5b98319

wfurt merged commit 3058899 into dotnet:master May 7, 2020

wfurt deleted the alerts branch May 7, 2020 20:46

karelz mentioned this pull request May 7, 2020

ServerAsyncAuthenticateTest.ServerAsyncAuthenticate_MismatchProtocols_Fails test failure #29642

Closed

This was referenced May 8, 2020

Ssl2 tests failing on old Windows in CI #36100

Closed

disable sslv2 tests for now #36102

Merged

ghost locked as resolved and limited conversation to collaborators Dec 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve handling of handshake failure #35549

improve handling of handshake failure #35549

wfurt commented Apr 28, 2020

ghost commented Apr 28, 2020

wfurt commented Apr 30, 2020

stephentoub May 5, 2020

wfurt May 6, 2020

stephentoub May 6, 2020

wfurt May 6, 2020

stephentoub May 6, 2020 •

edited

Loading

davidsh May 6, 2020

wfurt May 6, 2020

bartonjs May 6, 2020

wfurt May 7, 2020

wfurt commented May 7, 2020

wfurt commented May 7, 2020

improve handling of handshake failure #35549

improve handling of handshake failure #35549

Conversation

wfurt commented Apr 28, 2020

ghost commented Apr 28, 2020

wfurt commented Apr 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub May 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wfurt commented May 7, 2020

wfurt commented May 7, 2020

stephentoub May 6, 2020 •

edited

Loading